Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prefer Difflib in scrolling LiveText objects #12974

Merged
merged 7 commits into from Nov 4, 2021
Merged

Prefer Difflib in scrolling LiveText objects #12974

merged 7 commits into from Nov 4, 2021

Conversation

codeofdusk
Copy link
Contributor

@codeofdusk codeofdusk commented Oct 21, 2021 •
edited

Link to issue number:

Twitter thread.

Summary of the issue:

In LiveText objects that are constrained to onscreen text (i.e. where text scrolls off the screen), DMP is sometimes unable to correctly calculate new text, leading to choppy and inconsistent new text reporting.

Description of how this pull request fixes the issue:

Prefer Difflib in pre-FORMATTED UIA console (including legacy mode) and DisplayModelLiveText objects. Users can override this choice in advanced settings for testing or if DMP really provides a supperior experience in their situation.

Testing strategy:

Tested the following scenarios:

Application User setting Algorithm used
FORMATTED Windows Console Automatic DMP
FORMATTED Windows Console Diff Match Patch DMP
FORMATTED Windows Console Difflib Difflib
IMPROVED Windows Console Automatic Difflib
IMPROVED Windows Console Diff Match Patch DMP
IMPROVED Windows Console Difflib Difflib
END_INCLUSIVE Windows Console Automatic Difflib
END_INCLUSIVE Windows Console Diff Match Patch DMP
END_INCLUSIVE Windows Console Difflib Difflib
Legacy Windows Console Automatic Difflib
Legacy Windows Console Diff Match Patch DMP
Legacy Windows Console Difflib Difflib
Windows Terminal Automatic DMP
Windows Terminal Diff Match Patch DMP
Windows Terminal Difflib Difflib
PuTTY Automatic Difflib
PuTTY Diff Match Patch DMP
PuTTY Difflib Difflib

Known issues with pull request:

None known.

Change log entries:

== Bug Fixes ==

  • Improved consistency of output reading in terminal programs.
    • Note that in some situations, when inserting or deleting characters in the middle of a line, the characters after the caret may again be read out.

== Changes for Developers ==

  • diffHandler.get_dmp_algo and diffHandler.get_difflib_algo renamed diffHandler.prefer_dmp and diffHandler.prefer_difflib respectively.

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

@codeofdusk codeofdusk force-pushed the prefer-difflib-old-conhost branch 3 times, most recently from 21b7ee5 to fde3656 Compare October 21, 2021 20:26
@codeofdusk codeofdusk marked this pull request as ready for review October 21, 2021 20:53
@codeofdusk codeofdusk requested review from a team as code owners October 21, 2021 20:53
seanbudd
seanbudd requested changes Oct 27, 2021
View reviewed changes
source/NVDAObjects/behaviors.py Outdated Show resolved Hide resolved
source/NVDAObjects/behaviors.py Outdated Show resolved Hide resolved
source/NVDAObjects/window/winConsole.py Show resolved Hide resolved
source/NVDAObjects/window/__init__.py Show resolved Hide resolved
@seanbudd seanbudd self-assigned this Oct 27, 2021
codeofdusk and others added 2 commits October 27, 2021 00:20
@codeofdusk @seanbudd
Apply suggestions from code review
3d41706 
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
@codeofdusk
Review actions.
214d09e
seanbudd
seanbudd approved these changes Oct 27, 2021
View reviewed changes
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Qchristensen - what are your thoughts on the docs changes?

@AppVeyorBot
Copy link

See test results for failed build of commit 2bb94a9c51

@AppVeyorBot

This comment has been minimized.

Sign in to view
@AppVeyorBot

This comment has been minimized.

Sign in to view
Qchristensen
Qchristensen approved these changes Nov 4, 2021
View reviewed changes
Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, good work.

@seanbudd seanbudd merged commit fd9ae8c into nvaccess:master Nov 4, 2021
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanbudd seanbudd seanbudd approved these changes

@Qchristensen Qchristensen Qchristensen approved these changes

Assignees

@seanbudd seanbudd

Labels
None yet
Projects
None yet
Milestone
2022.1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@codeofdusk @AppVeyorBot @seanbudd @Qchristensen @nvaccessAuto